Skip to content

Available serialize enum #49

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Available serialize enum #49

wants to merge 3 commits into from

Conversation

peter-gribanov
Copy link
Contributor

See tests as how to usage

@mnapoli
Copy link
Member

mnapoli commented Mar 26, 2017

Hi, thank you for the pull request which looks very good. I have a few questions though:

  • what is the goal? Is it to avoid serializing the cached values?
  • will it break backward compatibility if anyone has serialized enums in the curent version, and then upgrades to a new version with your fix? (i.e. will it break?)

@peter-gribanov
Copy link
Contributor Author

peter-gribanov commented Mar 27, 2017

Received many complaints that it is impossible to serialize the list. Apparently, there is a demand for this.

An attempt to unserialize the list results in error:

Notice: unserialize(): Error at offset 54 of 65 bytes

@mnapoli
Copy link
Member

mnapoli commented Mar 27, 2017

It would be breaking the serialization format. So if anyone has stored (in database or anywhere else) serialized enums, that change would break everything that has been stored.

This is unfortunately a major BC break. Maybe there is a way to keep supporting the old serialization format?

@peter-gribanov
Copy link
Contributor Author

Yes. BC break. I add tests for check current serialize format #50

If you look, you will see that the output string is binary.
This can cause problems when it is stored in the database.

Perhaps you are right.
Perhaps this PR is not need.

@mnapoli
Copy link
Member

mnapoli commented Apr 8, 2017

Thank you for #50, it's now clearer.

I don't understand why it's serialized as a binary string but I guess it's not the real problem here.

So the big issue is backward compatibility with enums that have already been serialized by users and stored in database (for example). But you said:

An attempt to unserialize the list results in error

So right now is it possible to deserialize an enum?

If it's not possible it means serialization is broken and we can merge your pull request, it will not break an existing feature since serialization doesn't work.

@peter-gribanov
Copy link
Contributor Author

So right now is it possible to deserialize an enum?

I mistake. Now deserialize works. I serialized the enum to a file and successfully deserialized it.

@mnapoli
Copy link
Member

mnapoli commented Apr 11, 2017

Thanks, that confirms my quick tests too.

So I'm afraid we can't break backward compatibility :/

@peter-gribanov
Copy link
Contributor Author

In this case, i propose to conclude the discussion of this subject.
If you decide to break backwards compatibility, think about accepting this PR.
Thank you for your attention.

@mnapoli
Copy link
Member

mnapoli commented Apr 11, 2017

👍 thank you

@mnapoli mnapoli closed this Apr 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants